Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2965

OdedViner · 2025-01-12T12:51:45Z

The OCS operator used strings. Contains to match topology keys, leading to incorrect matches (e.g., topology.kubernetes.io/zone-principal instead of topology.kubernetes.io/zone). This caused invalid updates to nodeTopologies in the StorageCluster spec, breaking stretch cluster configuration and resulting in Rook operator failure.

Fix:
Updated the GetKeyValues method to explicitly match supported topology keys (rack, hostname, zone) using a predefined map. Unsupported or unmatched keys now return empty results.

Impact:

Prevents unintended matches.
Ensures correct StorageCluster and CephCluster configurations.
Improves topology handling reliability.

Nikhil-Ladha · 2025-01-13T06:45:39Z

api/v1/topologymap.go

+// GetKeyValues returns a node label matching the topologyKey and all values for StrechCluster
+// for that label across all storage nodes


Please update the comment as per the working of this function.

Nikhil-Ladha · 2025-01-13T06:46:11Z

api/v1/topologymap.go

+	for label, labelValues := range m.Labels {
+		if label == corev1.LabelZoneFailureDomainStable || label == labelZoneFailureDomainWithoutBeta {
+			topologyKey = label
+			values = labelValues
+			break
+		}
+	}


This means for stretch cluster we can't have a different failure domain? Like, rack, host?

We should update the GetKeysValues function to do an equality check with valid labels that we define in the validTopologyLabelKeys and return the key, values accordingly.

@malayparida2000 @iamniting what do you guys think?

Agree with @Nikhil-Ladha

Stretch always uses zones, just to confirm in this thread in addition to comments from others that confirmed the same.

malayparida2000

@OdedViner Can you add some details on what's the problem with the existing code? And what's the exact issue. It's difficult to understand the motive for the change without knowing the root cause you are trying to fix.

malayparida2000 · 2025-01-13T09:07:20Z

api/v1/topologymap.go

 	"strings"
 )

+const labelZoneFailureDomainWithoutBeta = "failure-domain.kubernetes.io/zone"


Wasn't this already defined somewhere in the codebase?

The variable is defined in another package within the project, but it is a private variable (starts with a lowercase letter). Since it cannot be accessed outside its package.

I saw our docs & went as far back as OCS 4.7. And I can confirm 2 things.

Stretch cluster always uses failure domain zone

For zone failure domain always topology.kubernetes.io/zone label is used. I don't see any need to search for any other labels such as failure-domain.kubernetes.io/zone.

Agreed with Malay on all these points. failure-domain.kubernetes.io/zone is an old deprecated label for topology that was removed from K8s many releases ago, we should not use it anymore.

OdedViner · 2025-01-13T11:39:21Z

@OdedViner Can you add some details on what's the problem with the existing code? And what's the exact issue. It's difficult to understand the motive for the change without knowing the root cause you are trying to fix.

Root Cause:
When incorrect labels are matched, it updates the NodeTopologies in the StorageCluster spec with incorrect zone information. This directly impacts the stretch cluster settings in the CephCluster spec by adding invalid zone information, causing the Rook operator to fail.

Impact:
1.The storage cluster misidentifies node topology information, leading to incorrect zone configuration.
2.In the stretch cluster setup, this results in the wrong zone labels being applied, disrupting the intended fault domain configuration.
3.Ultimately, the Rook operator fails to reconcile the CephCluster, breaking the deployment process.

Proposed Solution: To address this issue, I introduced a new function, GetKeyValuesStrechCluster, which specifically checks for relevant and valid topology keys (corev1.LabelZoneFailureDomainStable and labelZoneFailureDomainWithoutBeta). This ensures only the intended labels are considered, preventing incorrect matches and maintaining the integrity of the stretch cluster configuration.
The function isolates the logic required for stretch cluster scenarios, leaving the existing GetKeyValues behavior unchanged for other use cases. This makes the fix targeted and minimizes the risk of regressions elsewhere.

Why This Fix Is Necessary:
The root cause of the issue lies in the overly broad match logic (strings.Contains), which needs to be refined for stretch cluster use cases.
Without this fix, incorrect labels (e.g., topology.kubernetes.io/zone-principal) will continue to disrupt the stretch cluster settings.
The new function ensures correctness by limiting the keys checked to those explicitly relevant for stretch cluster configurations.
Let me know if more clarification or details are needed!

@malayparida2000 @Nikhil-Ladha @iamniting @sp98
Is the stretch cluster failure domain always set to "zone"?

malayparida2000 · 2025-01-13T12:05:51Z

@malayparida2000 @Nikhil-Ladha @iamniting @sp98 Is the stretch cluster failure domain always set to "zone"?
I belive so https://docs.redhat.com/en/documentation/red_hat_openshift_data_foundation/4.13/html/configuring_openshift_data_foundation_disaster_recovery_for_openshift_workloads/introduction-to-stretch-cluster-disaster-recovery_stretch-cluster#applying-topology-zone-labels-to-ocp-nodes_stretch-cluster
@travisn can you please correct me?

sp98 · 2025-01-13T12:15:13Z

Yes. It's always zone.

malayparida2000 · 2025-01-13T13:09:14Z

In the whole of ODF we have only 3 supported kind of failure domains rack, hostname, and zone.
rack- topology.rook.io/rack
hostname/host- kubernetes.io/hostname
zone- topology.kubernetes.io/zone
So I think we can just refactor the code base & stop caring about any other labels in here. This would help us eliminate the contains logic. And we can just directly match the label key with these 3 values of labels.

malayparida2000 · 2025-01-15T11:23:53Z

@OdedViner This fix selectively addresses the case of stretch cluster only, while the mis match of labels can still happen to a normal cluster if the customer uses an improperly formatted label. I would suggest fixing the issue properly for all cases.
Ref-#2965 (comment)
We should refactor the parts like how topologymap is built, failure domain is determined, keyvalues are fetched so that we finally fix this issue as a whole.

Restrict topology key matching to explicitly supported labels to prevent incorrect updates and ensure reliable cluster configuration. Signed-off-by: Oded Viner <[email protected]>

malayparida2000 · 2025-01-20T05:36:32Z

The fix looks good now, @OdedViner Can you please test this with a cluster & let us know the results here.

OdedViner · 2025-01-22T10:32:10Z

The fix looks good now, @OdedViner Can you please test this with a cluster & let us know the results here.

The private image seems to have resolved the issue

Procedure:
1.Deploy arbiter cluster:
ocs-ci path:
deployment/vsphere/upi_1az_rhcos_vsan_lso_vmdk_3m_6w_arbiter.yaml [4.18.0-109.stable image]

2.Add "topology.kubernetes.io/zone-principal=true" label to all worker nodes

oc label nodes compute-0 topology.kubernetes.io/zone-principal=true
oc label nodes compute-1 topology.kubernetes.io/zone-principal=true
oc label nodes compute-2 topology.kubernetes.io/zone-principal=true
oc label nodes compute-3 topology.kubernetes.io/zone-principal=true
oc label nodes compute-4 topology.kubernetes.io/zone-principal=true
oc label nodes compute-5 topology.kubernetes.io/zone-principal=true

Reset ocs-operaotr pod

4.Check Storagecluster status:

Message:                  Error while reconciling: CephCluster.ceph.rook.io "ocs-storagecluster-cephcluster" is invalid: spec.mon: Invalid value: "object": stretchCluster zones must be equal to 3

Failure Domain Key: topology.kubernetes.io/zone-principal

$ oc describe storagecluster
Status:
  Conditions:
    Last Heartbeat Time:      2025-01-21T11:54:01Z
    Last Transition Time:     2025-01-21T11:54:01Z
    Message:                  Version check successful
    Reason:                   VersionMatched
    Status:                   False
    Type:                     VersionMismatch
    Last Heartbeat Time:      2025-01-21T16:04:41Z
    Last Transition Time:     2025-01-21T15:29:45Z
    Message:                  Error while reconciling: CephCluster.ceph.rook.io "ocs-storagecluster-cephcluster" is invalid: spec.mon: Invalid value: "object": stretchCluster zones must be equal to 3
    Reason:                   ReconcileFailed
    Status:                   False
    Type:                     ReconcileComplete
    Last Heartbeat Time:      2025-01-21T15:29:06Z
    Last Transition Time:     2025-01-21T15:09:32Z
    Message:                  Reconcile completed successfully
    Reason:                   ReconcileCompleted
    Status:                   True
    Type:                     Available
    Last Heartbeat Time:      2025-01-21T15:29:06Z
    Last Transition Time:     2025-01-21T15:19:39Z
    Message:                  Reconcile completed successfully
    Reason:                   ReconcileCompleted
    Status:                   False
    Type:                     Progressing
    Last Heartbeat Time:      2025-01-21T15:29:06Z
    Last Transition Time:     2025-01-21T15:09:32Z
    Message:                  Reconcile completed successfully
    Reason:                   ReconcileCompleted
    Status:                   False
    Type:                     Degraded
    Last Heartbeat Time:      2025-01-21T15:29:06Z
    Last Transition Time:     2025-01-21T15:19:39Z
    Message:                  Reconcile completed successfully
    Reason:                   ReconcileCompleted
    Status:                   True
    Type:                     Upgradeable
  Current Mon Count:          5
  Default Ceph Device Class:  ssd
  Failure Domain:             zone
  Failure Domain Key:         topology.kubernetes.io/zone-principal

5.Create private image based relase-4.18 branch

$ export REGISTRY_NAMESPACE=oviner
$ export IMAGE_TAG=nitin-test
$ make ocs-operator
$ podman push quay.io/$REGISTRY_NAMESPACE/ocs-operator:$IMAGE_TAG

image location:
quay.io/oviner/ocs-operator:nitin-test

7.Change csv image

$ oc edit csv ocs-operator.v4.18.0-109.stable

                image: quay.io/oviner/ocs-operator:nitin-test
                imagePullPolicy: Always
                name: ocs-operator

Manual remove some entries from the StorageCluster CR status.

oc patch storagecluster ocs-storagecluster -n openshift-storage --type=json --subresource=status --patch '[

{ "op": "remove", "path": "/status/failureDomain" }
]'
oc patch storagecluster ocs-storagecluster -n openshift-storage --type=json --subresource=status --patch '[

{ "op": "remove", "path": "/status/failureDomainValues"}
]'
oc patch storagecluster ocs-storagecluster -n openshift-storage --type=json --subresource=status --patch '[

{ "op": "remove", "path": "/status/failureDomainKey" }
]'
oc patch storagecluster ocs-storagecluster -n openshift-storage --type=json --subresource=status --patch '[

{ "op": "remove", "path": "/status/nodeTopologies" }
]'

Reset ocs-operaotr pod

$ oc delete pod ocs-operator-74c9fd8477-kp88x 
pod "ocs-operator-74c9fd8477-kp88x" deleted

10.Check storagecluster status

Current Mon Count:          5
  Default Ceph Device Class:  ssd
  Failure Domain:             zone
  Failure Domain Key:         topology.kubernetes.io/zone
  Failure Domain Values:
    data-1
    data-2
  Images:
    Ceph:
      Actual Image:   registry.redhat.io/rhceph/rhceph-8-rhel9@sha256:665278442f848fe2ac12be483b8778e672f35325daf6b90977ac12c3165bcabc
      Desired Image:  registry.redhat.io/rhceph/rhceph-8-rhel9@sha256:665278442f848fe2ac12be483b8778e672f35325daf6b90977ac12c3165bcabc
    Noobaa Core:
      Actual Image:   registry.redhat.io/odf4/mcg-core-rhel9@sha256:35f7f2c455823b04f77f664d0acf0a5759d6c6601e9ae0ad11bb00b97ed8cf1c
      Desired Image:  registry.redhat.io/odf4/mcg-core-rhel9@sha256:35f7f2c455823b04f77f664d0acf0a5759d6c6601e9ae0ad11bb00b97ed8cf1c
    Noobaa DB:
      Actual Image:   registry.redhat.io/rhel9/postgresql-15@sha256:44a08b83a6c50714b52f4cf1c3476bc16b66faec21dd9a9bc07d1be5f97b8150
      Desired Image:  registry.redhat.io/rhel9/postgresql-15@sha256:44a08b83a6c50714b52f4cf1c3476bc16b66faec21dd9a9bc07d1be5f97b8150
  Kms Server Connection:
  Node Topologies:
    Labels:
      kubernetes.io/hostname:
        compute-0
        compute-1
        compute-2
        compute-3
        compute-4
        compute-5
      topology.kubernetes.io/zone:
        data-1
        data-2
      topology.kubernetes.io/zone-principal:
        true
  Phase:  Ready

$ oc describe csv ocs-operator.v4.18.0-109.stable | grep -i oviner -C 3
                  Value From:
                    Field Ref:
                      Field Path:   metadata.namespace
                Image:              quay.io/oviner/ocs-operator:nitin-test
                Image Pull Policy:  Always
                Name:               ocs-operator
                Readiness Probe:

malayparida2000

/lgtm

openshift-ci · 2025-01-22T10:36:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, malayparida2000, OdedViner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [iamniting]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

malayparida2000 · 2025-01-24T05:23:15Z

/retest

malayparida2000 · 2025-01-30T07:07:22Z

/cherry-pick release-4.18

openshift-cherrypick-robot · 2025-01-30T07:08:03Z

@malayparida2000: new pull request created: #2993

In response to this:

/cherry-pick release-4.18

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

OdedViner force-pushed the strech_cluster_zones branch from 9f8e66d to 2bf7271 Compare January 12, 2025 12:58

iamniting requested a review from malayparida2000 January 13, 2025 06:15

Nikhil-Ladha requested changes Jan 13, 2025

View reviewed changes

openshift-ci bot assigned Nikhil-Ladha Jan 13, 2025

malayparida2000 reviewed Jan 13, 2025

View reviewed changes

OdedViner force-pushed the strech_cluster_zones branch 3 times, most recently from 99f9f7d to 0796ed4 Compare January 14, 2025 17:14

OdedViner force-pushed the strech_cluster_zones branch from 0796ed4 to 8f85424 Compare January 15, 2025 15:09

OdedViner changed the title ~~Adjust Node Label Conditions Based on Full Label Name on strechcluster~~ Introduce GetKeyValuesStretchCluster for Accurate Label Matching in StretchCluster Jan 16, 2025

OdedViner force-pushed the strech_cluster_zones branch 10 times, most recently from cb85115 to 789e697 Compare January 18, 2025 19:00

OdedViner requested a review from Nikhil-Ladha January 18, 2025 19:04

OdedViner changed the title ~~Introduce GetKeyValuesStretchCluster for Accurate Label Matching in StretchCluster~~ Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap Jan 19, 2025

Restrict Node Label Matching for Supported Topology Keys

d60ecd7

Restrict topology key matching to explicitly supported labels to prevent incorrect updates and ensure reliable cluster configuration. Signed-off-by: Oded Viner <[email protected]>

OdedViner force-pushed the strech_cluster_zones branch from 789e697 to d60ecd7 Compare January 19, 2025 09:12

malayparida2000 approved these changes Jan 22, 2025

View reviewed changes

openshift-ci bot assigned malayparida2000 Jan 22, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2025

iamniting approved these changes Jan 22, 2025

View reviewed changes

openshift-ci bot assigned iamniting Jan 22, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2025

openshift-merge-bot bot merged commit 1085e40 into red-hat-storage:main Jan 24, 2025
11 checks passed

openshift-cherrypick-robot mentioned this pull request Jan 30, 2025

DFBUGS-160: [release-4.18] Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2965

Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2965

OdedViner commented Jan 12, 2025 •

edited

Loading

Nikhil-Ladha Jan 13, 2025

OdedViner Jan 14, 2025

Nikhil-Ladha Jan 13, 2025 •

edited

Loading

Nikhil-Ladha Jan 13, 2025

Nikhil-Ladha Jan 13, 2025

malayparida2000 Jan 13, 2025

travisn Jan 13, 2025

OdedViner Jan 14, 2025

malayparida2000 left a comment

malayparida2000 Jan 13, 2025

OdedViner Jan 13, 2025

malayparida2000 Jan 13, 2025

travisn Jan 13, 2025

OdedViner commented Jan 13, 2025

malayparida2000 commented Jan 13, 2025

sp98 commented Jan 13, 2025

malayparida2000 commented Jan 13, 2025 •

edited

Loading

malayparida2000 commented Jan 15, 2025

malayparida2000 commented Jan 20, 2025

OdedViner commented Jan 22, 2025

malayparida2000 left a comment

openshift-ci bot commented Jan 22, 2025

malayparida2000 commented Jan 24, 2025

malayparida2000 commented Jan 30, 2025

openshift-cherrypick-robot commented Jan 30, 2025

		// GetKeyValues returns a node label matching the topologyKey and all values for StrechCluster
		// for that label across all storage nodes

Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2965

Restrict Node Label Matching for Supported Topology Keys in NodeTopologyMap #2965

Conversation

OdedViner commented Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nikhil-Ladha Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malayparida2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OdedViner commented Jan 13, 2025

malayparida2000 commented Jan 13, 2025

sp98 commented Jan 13, 2025

malayparida2000 commented Jan 13, 2025 • edited Loading

malayparida2000 commented Jan 15, 2025

malayparida2000 commented Jan 20, 2025

OdedViner commented Jan 22, 2025

malayparida2000 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 22, 2025

malayparida2000 commented Jan 24, 2025

malayparida2000 commented Jan 30, 2025

openshift-cherrypick-robot commented Jan 30, 2025

OdedViner commented Jan 12, 2025 •

edited

Loading

Nikhil-Ladha Jan 13, 2025 •

edited

Loading

malayparida2000 commented Jan 13, 2025 •

edited

Loading